Hello @jonmmease and @nicolaskruchten, from my point of view this looks ready to go. I'd like to know what you think. The only failing tests are also failing for master. I have tweaked the docstrings and tests.

I'm used to being able to use pathlib everywhere, so I was surprised that plotly.py didn't support it.

What I find especially nice here is that it works with the pathlib.PurePath interface, and thus I can use it with packages like s3path.

Thanks for this PR, and sorry for the radio-silence! I'll review it soon... we're planning on releasing v5.0 of plotly sometime in April, and likely nothing between now and then, but I'll try to get this into the 5.0 release :)

I don't have time now, but it looks like one of the tests is failing in python-2.7-optional:

=================================== FAILURES ===================================
_______________________ test_kaleido_engine_write_image ________________________

    def test_kaleido_engine_write_image():
>       for writeable_mock in make_writeable_mocks():

plotly/tests/test_optional/test_kaleido/test_kaleido.py:86: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
plotly/tests/test_optional/test_kaleido/test_kaleido.py:44: in make_writeable_mocks
    mock_pathlib_path.active_write_function = mock_pathlib_path.write_bytes
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <Mock spec='Path' id='140019672186256'>, name = 'write_bytes'

    def __getattr__(self, name):
        if name in ('_mock_methods', '_mock_unsafe'):
            raise AttributeError(name)
        elif self._mock_methods is not None:
            if name not in self._mock_methods or name in _all_magics:
>               raise AttributeError("Mock object has no attribute %r" % name)
E               AttributeError: Mock object has no attribute 'write_bytes'

.tox/py27-optional/lib/python2.7/site-packages/mock/mock.py:698: AttributeError
____________________ test_kaleido_engine_write_image_kwargs ____________________

    def test_kaleido_engine_write_image_kwargs():
>       for writeable_mock in make_writeable_mocks():

plotly/tests/test_optional/test_kaleido/test_kaleido.py:119: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
plotly/tests/test_optional/test_kaleido/test_kaleido.py:44: in make_writeable_mocks
    mock_pathlib_path.active_write_function = mock_pathlib_path.write_bytes
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <Mock spec='Path' id='140019760188304'>, name = 'write_bytes'

    def __getattr__(self, name):
        if name in ('_mock_methods', '_mock_unsafe'):
            raise AttributeError(name)
        elif self._mock_methods is not None:
            if name not in self._mock_methods or name in _all_magics:
>               raise AttributeError("Mock object has no attribute %r" % name)
E               AttributeError: Mock object has no attribute 'write_bytes'

.tox/py27-optional/lib/python2.7/site-packages/mock/mock.py:698: AttributeError

That's looking much better now. I'm fairly confident that the failing tests are not caused by my changes.

Please let me know if I should squash or rebase.

@jonmmease thoughts on making pathlib a dependency here?

pathlib is built-in since 3.4, so technically the only external "dependency" would be pathlib2 for legacy.

Gotcha. We're dropping support for Python < 3.6 actually in the next release so this pathlib2 dependency won't be necessary then :)

If you can remove the pathlib2 stuff and merge master into your branch, the CI should be much cleaner now :)

Hello @nicolaskruchten, thanks so much for the review! As requested, I have rebased and removed the legacy support. As far as I can tell, the failing tests are not my fault.

Hmmm are you sure you rebased onto the latest master? the name of some of those CI jobs are out of date :)

@nicolaskruchten ya, master on my fork! 😝 Thank you!!!

Give me another minute and I'll fix write_html as well...

Thanks! I think if you're on a roll, write_json might benefit from the same treatment?

Ah, and I just realized that the actual implementation of the sort-of abstract write_image function exists both in io/_kaleido.py where you made your changes but also in io/_orca.py (the older way of exporting static images) so ideally we'd make these changes there too for consistency.

(thanks so much for all the work on this, I really appreciate it :) )

@nicolaskruchten, thank you for writing amazing open-source software!!!

Hopefully that does it. I'm not terribly pleased with all the code duplication, but I'm not so sure how you would want to refactor it. (There was already similar duplication in the original code.)

This is awesome, thank you, and thanks for thinking of read_json even though I forgot it!

I'll poke at it a little bit more locally but this looks good to merge shortly, and certainly we'll get these changes in to v5 when it comes out in a couple of weeks. The duplication is a bit gross indeed but it's not your fault you didn't make anything worse ;)

Note: I couldn't find the proper place to write pathlib tests for orca, so I didn't write any. I'm not sure how much you care? I like kaleido way better!!! (Did you choose the name just so you could make the pun with "scope"? 😆 If so, that's true dedication!)

(Did you choose the name just so you could make the pun with "scope"? 😆 If so, that's true dedication!)

I kind of think that's why @jonmmease chose it, yes :P

Looks awesome, thanks again :)